Skip to content

common/hexutil: hex encode/decode in simd - #24410

Open
AskAlexSharov wants to merge 32 commits into
mainfrom
alex/simdhex_37
Open

AskAlexSharov wants to merge 32 commits into
mainfrom
alex/simdhex_37

Conversation

@AskAlexSharov

@AskAlexSharov AskAlexSharov commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Reason: reth using const-hex, neth using Core/Extensions/HexEncoder.cs (encode: AVX-512 VBMI/AVX2/SSSE3/NEON), HexConverter.cs (decode: SSSE3/NEON)

Hex encoding and decoding for RPC byte fields in assembly: AVX2 on amd64, NEON on arm64. amd64 without AVX2 and other architectures use encoding/hex. No GOEXPERIMENT needed.

The first version used simd/archsimd (up to 731ff44). It needs GOEXPERIMENT=simd, which slows all Go code on AVX-512 CPUs (golang/go#81847: struct copy +26%), so it could not ship in release builds.

ns per call (lower is better), n5 (AMD EPYC 4344P) and M4 Max; simd is the archsimd kernel:

            n5 encode          n5 decode          M4 encode         M4 decode
size     asm  simd  stdlib   asm  simd  stdlib   asm  simd  stdlib  asm  simd  stdlib
20       7.3   5.1   13.2    8.7   7.3   11.9    5.4   4.0   12.1   7.1   5.8   12.9
32       5.2   3.6   20.2    7.3   6.6   18.2    4.0   2.5   18.2   5.6   4.8   18.8
64       5.9   4.2   39.0    8.5   9.2   35.0    4.8   3.8   34.1   7.1   7.0   35.6
256     10.0  11.0  156.6   16.2  24.4  138.9   11.0  11.3  135.1  21.4  22.4  143.5
1 KiB   29.0  39.3  612.8   50.6  86.4  535.7   30.5  41.1  514.9  83.9  88.7  553.7
110 KB  2778  4142  66258   5093  8876  57573   2690  4328  55226  9018 10352  59769

Assembly is never async-preempted, so hex_asm.go feeds it at most 64 KiB per call. That alone is not enough: the assembler drops the stack check of a leaf function with a frame under 128 bytes, so the kernels declare a 128-byte frame, and the check is where a pending preemption stops the goroutine between chunks. Worst GC stop-the-world wait while 64 MiB goes through, GOMAXPROCS=2: 5-6 ms in one call, 0.012-0.016 ms in 64 KiB chunks.


Decoding too, by algorithm 3 of http://0x80.pl/notesen/2022-01-17-validating-hex-parse.html (as const-hex in alloy/reth): a digit maps to 0-9 and a letter of either case to 10-15 on two saturating paths, anything else to more than 15 on both, so the smaller of the two is the nibble, and one VPMADDUBSW merges each pair. A block holding a character that is not a hex digit falls to hex.Decode, which reports it with the same error.

It is 18.9% of an eth_call replaying mainnet tx 0x8653e24402c22383aaf3c92a9b08307ac4bada805eef06bd945bedb639af7393 (block 26063051, 110.8 KB of calldata), reached from hexutil.(*Bytes).UnmarshalText.

arm64 too, on NEON: encode by VTBL table lookup, 16 bytes per iteration; decode by the same algorithm, 32 characters per iteration, packed with VUZP1/VUZP2.

Both AVX2 kernels end with VZEROUPPER, as Go CL 795820 does: the Go code after them is SSE, which pays a false dependency on Intel while the upper halves of the Y registers are dirty.

@AskAlexSharov AskAlexSharov changed the title common/hexutil: AVX2 and SWAR hex encoding common/hexutil: AVX2 hex encoding Sep 30, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Standard CI does not compile or exercise the new SIMD implementation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds optional AVX2 acceleration for RPC hex encoding while retaining the standard-library fallback.

Changes:

  • Introduces SIMD and generic encoding kernels.
  • Routes byte encoding APIs through the shared kernel.
  • Adds correctness tests and benchmarks.
File Description
common/​hexutil/​hexutil.go Routes Encode through Bytes.AppendText.
common/​hexutil/​hexutil_test.go Adds encoding equivalence tests.
common/​hexutil/​hexutil_bench_test.go Benchmarks quoted hex encoding.
common/​hexutil/​encode_simd.go Implements the AVX2 encoding kernel.
common/​hexutil/​encode_generic.go Provides the standard-library fallback.
common/​hexutil/​bytes.go Uses the shared kernel for byte encoding.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread common/hexutil/hexutil_test.go
The regular jobs use the Go version from go.mod and default experiments, so they never compile encode_simd.go.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The implementation relies on experimental Go 1.27 SIMD intrinsics and CPU-specific runtime behavior requiring final human validation.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

@AskAlexSharov AskAlexSharov changed the title common/hexutil: AVX2 hex encoding common/hexutil: hex encoding by simd Sep 30, 2026

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kernel verified: I built the head with go1.27.1 + GOEXPERIMENT=jsonv2,simd for amd64 and ran it under Rosetta 2 (which reports AVX2): common/hexutil passes, also with -race and with GODEBUG=cpu.avx2=off; rpc/jsonstream, common and execution/types pass too. Four hand-made kernel mutants (table digit, nibble mask, shift width, dst stride) all fail TestEncodeHexMatchesStdlib. The vector loop has no bounds checks in the disassembly, as the commit says. No correctness issue.

Two things I'd like fixed before merge, one small regression, and a few notes.

1. encodeHex returns with dirty YMM upper halves (no VZEROUPPER)

go1.27.1 does not emit VZEROUPPER for archsimd code (checked with objdump), and archsimd.ClearAVXUpperBits exists for exactly this; every AVX2 routine in the Go runtime and bytealg ends with VZEROUPPER for the same reason. After encodeHex the rest of the request runs legacy-SSE code (memmove, MOVUPS copies). On Skylake and later each SSE register write pays a merge µop with a false dependency until the next VZEROUPPER, which in practice is the next memmove over 256 bytes; on Haswell/Broadwell it is a ~70-cycle state transition each way, more than the ~15 ns the kernel saves per hash. AMD has no penalty, which is why the Zen 4 numbers don't show it. Suggested shape (tested; one extra instruction after the loop, and short inputs no longer touch YMM at all):

	if hasAVX2 && len(src) >= 16 {
		digits := archsimd.LoadUint8x32Array(&hexDigits32)
		low := archsimd.BroadcastUint16x16(0x0f)
		for len(src) >= 16 && len(dst) >= 32 {
			...
		}
		archsimd.ClearAVXUpperBits()
	}
	hex.Encode(dst, src)

#24411 and #24418 have the same gap.

2. test-simd.yml as a required merge-queue leaf

It becomes a required ci-gate job, but skips everything setup-erigon does to keep module fetching deterministic: cache: false with no replacement, so every run downloads 32 modules (see the job log), no GOPROXY=https://proxy.golang.org|direct fallback and no retry loop. That is a new flake vector in the merge queue, which CI-GUIDELINES asks to keep free of false positives. It also lacks the "Cancel workflow run on failure" step (and the actions: write permission it needs) that the other leaves use to fast-evict a broken PR, and it runs an inline go test with no Makefile equivalent.

Suggestion: add an optional go-version input to setup-erigon (default stays the go.mod minor) and use it here, so the job inherits the mod cache, proxy fallback, retries and cache keys; register it in cache-warming.yml like test-bench; add a make test-simd target with GOEXPERIMENT=jsonv2,simd that the workflow calls. When #24411/#24418 land the package list needs extending; deriving it from grep -rl goexperiment.simd --include='*.go' would keep it in one place.

3. Default build: Encode / MarshalText are slower for small inputs

Bytes(b).AppendText(nil) goes through slices.Grow(nil, …) → growslice, while the old make([]byte, n) is stack-allocated for n ≤ 32 on Go ≥ 1.25 and cheaper than growslice above that. Default build (arm64, go1.27.1, -cpu 1, n=8), main → this PR: Encode 4/8/15 B: 21.5→35.5 ns, 23.6→38.6 ns, 28.4→45.1 ns (1→2 allocs); Encode 20/32 B: +6%; MarshalText 4–15 B: +23–28%, 20/32 B: +16%/+10%; ≥64 B unchanged. AppendText/AppendQuoted (the RPC path) are unchanged. Keeping make in MarshalText and sharing the prefix write brings both back to main or slightly better (measured):

func (b Bytes) MarshalText() ([]byte, error) {
	result := make([]byte, len(HexPrefix)+2*len(b))
	writeHex(result, b)
	return result, nil
}

func (b Bytes) AppendText(dst []byte) ([]byte, error) {
	n, size := len(dst), len(HexPrefix)+2*len(b)
	dst = slices.Grow(dst, size)[:n+size]
	writeHex(dst[n:], b)
	return dst, nil
}

func writeHex(dst, b []byte) {
	dst[0], dst[1] = '0', 'x'
	encodeHex(dst[2:], b)
}

func Encode(b []byte) string {
	enc, _ := Bytes(b).MarshalText()
	return string(enc)
}

Notes, non-blocking

  • As merged, no shipped binary gets the kernel: the Makefile sets GOEXPERIMENT ?= jsonv2 and the Dockerfile builds through make, so the RPC numbers in the description only apply to GOEXPERIMENT=jsonv2,simd builds. Worth one sentence in the description (and the ChangeLog, if it gets an entry) so nobody expects them from a release.
  • TestEncodeHexMatchesStdlib passes on a runner without AVX2 without ever running the kernel. A require.True(t, hasAVX2) in encode_simd_test.go when CI is set would make that visible.
  • Description typo: the package is simd/archsimd, not simd/simdarch.

@AskAlexSharov
AskAlexSharov removed the request for review from chfast October 1, 2026 12:07
The self-hosted ARM64 label is the one release.yml's test-release holds
for up to two days, and ci-gate calls lint.yml on every PR and merge
group, so that job would have blocked the queue behind a release test.

Also drop two steps that covered nothing: with the feature bit cleared
these functions are hex.Encode and hex.Decode, and the cross-build is
what the two test runs and every non-simd build already do.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The ISA test currently runs under the normal Go 1.26 amd64 suite despite requiring the Go 1.27 SIMD experiment.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread common/hexutil/isa_test.go Outdated
go.mod pins go 1.26, so the ordinary amd64 and macOS suites compiled
this test and then failed starting that toolchain with GOEXPERIMENT=simd,
which needs 1.27. It now carries the kernel's own build tag.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The experimental architecture-specific kernels require human review, and the arm64 CI job does not persist its caches.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity arm64 CI job never saves dependency cache

.github/​workflows/​lint.yml:174

setup-erigon only restores caches; cleanup-erigon performs the save. This new job never runs cleanup, so the new arm64 namespace stays cold and every CI run must download and compile its dependencies again. Add the same always-running cleanup step used by the other workflow jobs.

With a second job in the workflow, zizmor reports the workflow-level
grant as excessive-permissions at high severity, which takes its exit
code past the threshold the step tolerates. Only the self-cancelling
step needs it.
… block

Encode uses VTBL (LookupOrZero), 16 bytes per iteration. Decode uses Muła/Langdale algorithm 3.
…Muła/Langdale algorithm 3

Both kernels clear the upper Y halves on exit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Architecture-specific experimental SIMD kernels require final human review, and the new ARM job does not persist its caches.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Medium severity Go cache is never saved because cleanup-erigon is missing

.github/​workflows/​lint.yml:178

This job restores uniquely namespaced Go caches through setup-erigon, but never runs cleanup-erigon, so no ARM cache is saved and every run must download and rebuild dependencies again. Other setup-based jobs pair the actions (for example, .github/workflows/test-integration-caplin.yml:32-57). Add the cleanup action with if: always() after the test.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Same algorithms as the archsimd kernels, without GOEXPERIMENT=simd, so default builds use them. A 128-byte frame keeps the stack check, which lets GC stop the goroutine between chunks. The simd CI steps go; test-all already runs common/hexutil on macos-15 (arm64).
@AskAlexSharov AskAlexSharov changed the title common/hexutil: hex encoding by simd common/hexutil: AVX2 and NEON hex encoding in assembly Oct 8, 2026
@AskAlexSharov AskAlexSharov changed the title common/hexutil: AVX2 and NEON hex encoding in assembly common/hexutil: hex encode/decode in simd Oct 8, 2026
AppendText(nil) grows through slices.Grow, which always allocates on the heap. MarshalText makes its buffer again, and Encode builds on it, so a short encoding stays on the stack.
Drops the packBytes table. The decode benchmark loses its encoding/hex arm.
Go 1.26 has no UQSUB, UQADD or UMAXV in its arm64 assembler, which broke the macOS build.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The handwritten dual-architecture assembly requires final human validation despite comprehensive equivalence tests.

0 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@AskAlexSharov
AskAlexSharov marked this pull request as ready for review October 8, 2026 03:16
@AskAlexSharov
AskAlexSharov requested a review from chfast October 8, 2026 03:16
@mauri870

mauri870 commented Oct 8, 2026 •

Copy link
Copy Markdown

@AskAlexSharov Have you considered upstreaming your encoding/hex vectorized implementation to the Go standard library? I would be happy to review the CL.

@mauri870

mauri870 commented Oct 8, 2026

Copy link
Copy Markdown

go1.27.1 does not emit VZEROUPPER for archsimd code (checked with objdump), and archsimd.ClearAVXUpperBits exists for exactly this; every AVX2 routine in the Go runtime and bytealg ends with VZEROUPPER for the same reason. After encodeHex the rest of the request runs legacy-SSE code (memmove, MOVUPS copies).

That looks like golang/go#80835.

@AskAlexSharov

Copy link
Copy Markdown
Collaborator Author

@AskAlexSharov Have you considered upstreaming your encoding/hex vectorized implementation to the Go standard library? I would be happy to review the CL.

Thank you for invitation. Done: golang/go#82089

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants